Introduce token modes - #127
Conversation
msujew
left a comment
msujew
left a comment
There was a problem hiding this comment.
Nice! Can you resolve the merge conflict so we can check the performance in the GitHub CI? Locally, it seems to look good. Only a 2-3% loss on the statemachine example, which is acceptable.
|
@msujew I discovered a problem with my main grammar. The problem arises when I create a token mode and start to create token declarations within this token mode. As soon as I complete the word "token", the language server seems to end up in a endless loop. It does not react anymore. After completing the declaration and restarting the LS, it works. So the problem is during editing incomplete grammars :-/... The rule If I would refactor the grammar to have no common prefix in the subrules of Before I refactor it would be good to discuss whether these members have a good syntax or whether we should alternate it (which could remove the common prefix automatically). |
|
Ideas... Some quick fixes or code actions come into my mind:
Or
|
Does this require some setup? I could not reproduce this: 
I would keep it as it is for now 👍 |
467be1d to
2996cda
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 1ee2bcd | Previous: d803417 | Ratio |
|---|---|---|---|
BenchmarkWorkspaceCycle (typefox.dev/fastbelt/examples/statemachine) - MB/s |
11.92 MB/s |
5.21 MB/s |
2.29 |
This comment was automatically generated by workflow using github-action-benchmark.
No, it should fail out of the box. I can also complete this line as you started. But afterwards EDIT: I found out how to debug. Some kind of endless-loop in entered in this case. EDIT: Fixed the endless loop and another issue with the help of @msujew |
Lotes
left a comment
Lotes
left a comment
There was a problem hiding this comment.
Added some findings and also questions from my side.
msujew
left a comment
msujew
left a comment
There was a problem hiding this comment.
Really solid contribution. Thanks a lot!
I noticed another few missing validations that we should add as part of this PR:
- Duplicate keyword/token definitions in the same token mode should result in an error. I.e. you can currently write
token mode default { "x" "x" }. - Similarly Token references should be unique in the same token mode. Right know, you can write something like
token mode default { hidden WS comment WS }without a validation error appearing. - When a token is declared, but not referenced in any token mode, the diagnostic appears on the first usage site. I would expect it on the token declaration itself. Also, similar to the keyword issue, this should be an error.
- A non-default token mode without a
popcommand should show a diagnostic.
# Conflicts: # internal/grammar/types_gen.go # Conflicts: # internal/grammar/types_gen.go
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io> # Conflicts: # lexer/lexer.go
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
17d7c0e to
557bdc7
Compare
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
msujew
left a comment
msujew
left a comment
There was a problem hiding this comment.
Looking closer at some of the logic, I found some issues with some specific constellations. Works much better in general now, though!
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
There was a problem hiding this comment.
Good job @Lotes,
did a medium deep dive into this PR, Claude (Sonnet) did a much deeper one (the long headlined comments).
I like the tests you contributed a lot. However, esp. in the util tests I had trouble to get what the tested behavior is supposed to be.
Please add some docs, I left some remarks.
If the remarks provided by Claude don't make sense, feel free to ignore them.
(Maybe it's worth a comment, why they're invalid?)
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
sailingKieler
left a comment
sailingKieler
left a comment
There was a problem hiding this comment.
Thanks @Lotes for your updates.
I have a few more remark & questions for the sake of making things clear and precise.
| // Actually it would be sufficient to store one array of token type indices, but | ||
| // we split this set by category: keywords and regexp tokens. Keywords are matched first, | ||
| // so they have priority over regexp tokens. | ||
| type TokenTypeIndices struct { |
There was a problem hiding this comment.
Now that you have clarified it:
Is there a specific benefit in extracting (wrapping) the Keywords and Tokens slices into this extra struct type?
Looking at the references you almost always reference them like tokenMode.TokenTypeIndices.Keywords and tokenMode.TokenTypeIndices.Tokens.
This TokenTypeIndices type feels a bit artificial to me, I prefer to just inline them in TokenMode as long as there's no specific reason to have them like this.
There was a problem hiding this comment.
It IS an artificial type. Its role is mental grouping. Both sub elements have the same task for different entities of the grammar.
I also did this for TokenIndexLookup or TokenTypeLookup. Their purpose is grouping the mental load and improve readability.
From this perspective I would keep this separation :-) .
Or do you think this might have an impact on the performance?
There was a problem hiding this comment.
Okay that makes sense. Referring to what Miro mentioned in the meeting today: Please make this transparent in the docs locally, ideally on each type, since nobody knows your personal minds.
The problem coming up with such groupings is that you need additional names, and the name TokenTypeIndices suggests something big or important that it actually isn't here.
What about calling it just Tokens or ModeTokens?
Here I would even tend to make it private to emphasize that this type is just a vehicle rather than a first class citizen.
| // (which is something like the token rule name). Especially for | ||
| // keywords which can be standalone or part of a token declaration, | ||
| // this is important to know which token index is used at runtime | ||
| // (so: two entries can have the same token index). |
There was a problem hiding this comment.
How can this be?
In TokenType you stated Token index is the unique ID at runtime
There was a problem hiding this comment.
// Example why this is important:
//
// grammar XY
// entry Start: "a" A;
// token A: "a"
//
// There are two token types: one for the keyword "a" and one
// for the token declaration A. But since the content is the same,
// they share the same token index. So the generated code will
// have two token types (Keyword_a and Token_A) with the same token
// index, but only one of them will be used as default.
To be honest, this decision felt weird. Would you prefer a different solution?
There was a problem hiding this comment.
Now that I see this - we should probably validate against this. Separate ways of declaring tokens probably leads to a lot of headache. If a keyword token is already defined, a "simple" declaration of the same keyword should be forbidden.
There was a problem hiding this comment.
Would you prefer a different solution?
The design decisions are up to you guys.
I just aimed at bringing up things of which I struggled with while reviewing and confusing things.
There was a problem hiding this comment.
Please refine this doc then:
In
TokenTypeyou statedToken index is the unique ID at runtime
| for _, tokenType := range r.TokenTypes.All { | ||
| if tokenType.TokenIndex > maxId { | ||
| maxId = tokenType.TokenIndex | ||
| } |
There was a problem hiding this comment.
So maxId will be !== len(r.TokenTypes.All)?
There was a problem hiding this comment.
According to the comment above: Some Token declarations and keywords share the same token index: So maxId might be smaller than the number of all token types.
Maybe we can find a better naming or a solution around this hack. It feels confusing, but made sense to me during the time of implementation...
| // Tokens is the main token stream passed to the parser. | ||
| Tokens []core.Token | ||
| // Comments holds tokens whose [core.TokenType] uses [core.CommentGroup]. | ||
| // Comments holds tokens whose [core.TokenType] uses [core.CommentModifier]. |
There was a problem hiding this comment.
... in which way?
As ID? Why then the name core.CommentModifier (aliasing the value of 0x11111...0)?
There was a problem hiding this comment.
Modifiers have the role of an ANTLR channel I think (piping categorized tokens in a own array). But the name does more reflect the aspect of visibility to the user.
All tokens of a specific modifier will be placed in a special array, like Comments.
The value -2 was chosen for comments. Hidden is -1 and , I think, 0 is the visible output channel. You could also use different numbers here for extra channels. Do not ask me how ^^*...
There was a problem hiding this comment.
What I wanted to bring up: The statement
// Comments holds tokens whose [core.TokenType] uses [core.CommentModifier].
causes more confusion rather than clarity / more questions than answers. Please refine.
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
3 participants
Closes #50
push(name)to push a token mode onto the token mode stackpopto pop the topmost token modemode(name)to set the topmost token modeKeywordSelector